Skip to content

test(cketh): build the live balance-scan harness on the shared fixtures - #11124

Merged
gregorydemay merged 31 commits into
masterfrom
ic_DEFI-2262_8_live-scan-setup
Aug 18, 2026
Merged

test(cketh): build the live balance-scan harness on the shared fixtures#11124
gregorydemay merged 31 commits into
masterfrom
ic_DEFI-2262_8_live-scan-setup

Conversation

@gregorydemay

@gregorydemay gregorydemay commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

The live-anvil balance-scan harness had grown into a second test fixture: it created and funded canisters, sequenced their installs, and registered ckERC20 tokens on its own, in parallel with the fixtures every other cketh integration test already uses. This PR removes that duplication — the harness now builds on the shared fixtures and keeps only what is genuinely specific to it: the local Ethereum node it owns, and the balance-scan assertions.

Collapsing the two fixtures meant removing the knobs the duplicate had accumulated:

  • Whether PocketIC runs live is decided once, after construction, not by the chain under test. Every fixture — mocked or live — builds on an ordinary, non-live PocketIC instance, where setup calls tick deterministically; only LiveBalanceScanSetup::new_live switches the instance to auto-progress, and only once its whole fixture — including the ledger-suite orchestrator and the tokens it registers — is already built. Building against a live (wall-clock-paced) instance from the start made every setup call race a round deadline it didn't control, which reproducibly failed under CPU contention.
  • The harness owns the local Ethereum node rather than borrowing a URL to it, so the type proves a node is really running behind that URL.
  • Callers can no longer supply a pre-built environment. The chain under test already determines the canisters' init args, and every caller was passing exactly what would have been built anyway — the parameter only offered a way to get it wrong.
  • Canisters are created and installed in the same order for every chain under test, as on master; only their init args differ. The harness' dedicated controller and its skipped ledger install turned out not to be load-bearing: the anonymous principal already controls everything PocketIC creates by default, and the skipped install was down to a Bazel data dependency that simply wasn't declared.
  • Token registration goes through a real ledger-suite orchestrator instead of the harness impersonating one with placeholder ledgers — which is what testing the deposit flow will need next.
  • The set of tokens the balance scan credits with anvil bytecode is now exactly the set the orchestrator registered, not a separately hardcoded list — Holding and the harness' token-facing methods take the orchestrator's own token type directly, so the two can no longer silently drift apart.

Behaviour of the mocked fixtures is otherwise unchanged: same canister creation order, same derived minter address, and the same cold-start property the deposit-flow tests depend on. One thing did change for every fixture, mocked included: canisters are now funded with u64::MAX cycles rather than u128::MAX — the amount that reproducibly crashes the live harness' replica on its first HTTPS outcall — deliberately funding every fixture the same way rather than carrying two cycle amounts. No cketh test asserts on cycle balances, so this is not expected to be observable.

🤖 Generated with Claude Code

Shares the EVM RPC/minter install-args construction between the mocked
CkEthSetup fixture and the live anvil-backed balance-scan harness via
a new EvmRpcBackend enum, instead of live_scan.rs duplicating those
functions wholesale. Also extracts canister creation and ledger install
out of CkEthSetup::new into reusable helpers, and fixes the module doc
comment, which predated the ckerc20 mocked fixture's migration to
PocketIC and still claimed it ran on StateMachine.

The live harness keeps its own canister-creation, cycles and
non-anonymous controller setup separate from CkEthSetup::new: converging
it onto the shared (anonymous-controller) construction reproducibly
crashed the PocketIC replica with a cycle-accounting assertion failure
once the minter went live, so that axis stays intentionally distinct.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the ckETH/ckERC20 PocketIC test fixtures to reduce duplicated canister setup/installation logic, while keeping the live (anvil-backed) harness’s intentionally distinct controller/cycles behavior.

Changes:

  • Extracts shared canister creation + ckETH ledger installation into reusable helpers.
  • Introduces an EvmRpcBackend (Mocked vs Anvil) to centralize EVM RPC provider override + block-height/last-scraped assumptions.
  • Updates the live balance-scan harness to reuse the shared PocketIC builder chain and refreshes its module documentation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
rs/ethereum/cketh/test_utils/src/live_scan.rs Updates live scan harness docs and reuses shared builder + EvmRpcBackend-driven install configuration.
rs/ethereum/cketh/test_utils/src/lib.rs Extracts shared canister creation/ledger install helpers and adds EvmRpcBackend + pocket_ic_builder() for reuse across fixtures.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs
Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
@gregorydemay

Copy link
Copy Markdown
Contributor Author

🤖🧐 VERDICT: CHANGES_REQUESTED — 0 blockers, 2 mediums, 4 nits; CI pending (Build IC, Bazel Test All, Bazel Test All on RBE still running; nothing red).

Review details

Verified claims

  • CkEthSetup::new is behaviourally unchanged. Compared operation for operation: same three create_canister calls in the same order (minter first, so the hard-coded canister id / Ethereum address still hold), same u128::MAX cycles, same ledger init args, same installs in the same sequence, no added tick(). EvmRpcBackend::Mocked.install_args() is byte-equal to the previous InstallArgs::default() — every InstallArgs field is an Option and the struct derives Default, so the explicit override_provider: None changes nothing. The fixture's cold-start property (no IC rounds executed in new) is preserved.
  • The live harness's ordering invariant survives. make_live(None) still runs before install_minter (live_scan.rs:119:121) and after install_evm_rpc (:111), and the comment explaining why (install schedules timers whose outcalls would stall holding task guards) is retained verbatim.
  • The module doc fix is correct. CkErc20Setup has run on PocketIC since test(cketh): migrate integration tests to PocketIC #10955; the new wording ("also runs on PocketIC but answers ... with canned canister-http mocks") states the real distinction.

The three axes kept separate

  • Controller / settings / cycles — keeping this separate is right, but for a reason the code does not state. See the inline comment on live_scan.rs:74: the Invalid cycle change: expected Added(1360000000), got Added(0) signature points at the shared helper's add_cycles(id, u128::MAX) saturating the balance, not at the anonymous controller. Worth a one-line re-test before the comment is frozen into the tree.
  • Ledger placeholder — justified as written. Adding LEDGER_CANISTER_WASM_PATH to the deposit_from_cex target to install a ledger the balance-scan path never calls buys nothing.
  • ckERC20 activation via a bare upgrade_canister — justified. Stop/start exists in the mocked fixture to quiesce mocked outcalls; live outcalls are real, so there is nothing to quiesce.

Maintainability accounting

  • Duplication: found — install_minter/install_evm_rpc remain duplicated across the two files after the refactor (🟠, inline). Also a minor regression: the ECDSA_KEY_NAME constant was replaced by a literal now written twice (🔵).
  • Structural duplication against the codebase: the mirrored pair above is the only instance; no other sibling type is being paralleled.
  • Unused derives: none — the PR adds no derives.
  • Primitive-obsession parameters: cleared. EvmRpcBackend::Anvil(&str) matches the surrounding fixture style (anvil.url() already returns &str).
  • Divergent invariant handling: one instance — the two fixtures fund canisters with different cycle amounts (u128::MAX vs u64::MAX) with no stated rule for which applies when. Folded into the 🟠 on live_scan.rs:74.
  • Silent fallbacks: none found.
  • Test-only code in a production module: N/A — the whole crate is test_utils.
  • Redundant / derivable parameter: backend is constant (Mocked) at both lib.rs call sites; folded into the 🟠/🔵 pair on the duplication and the enum.
  • Decision the caller should not own: the install sender — pushing it into the shared installers is what unlocks deleting the duplicate copies.
  • Docs/comments: three findings (🟠 on the crash rationale, 🔵 on the uncommented Mocked arms, 🔵 on the CkEthCanisters doc). No JIRA refs, no metadata, no commented-out code.

Testing

No behaviour change, so no new test is owed; the existing suites are the regression net and all pass locally:

  • //rs/ethereum/cketh/minter:integration_tests_tests/cketh_test PASSED (69.4s)
  • //rs/ethereum/cketh/minter:integration_tests_tests/ckerc20_test PASSED (132.5s)
  • //rs/ethereum/cketh/test_utils:lib_tests PASSED
  • //rs/ethereum/cketh/minter:deposit_from_cex PASSED (64.8s) — the live PocketIC + anvil harness this PR rewires
  • cargo clippy --all-targets --all-features -p ic-cketh-test-utils -p ic-cketh-minter -- -D warnings clean

All run with --nocache_test_results.

Carries an install/upgrade sender (Option<Principal>) on CkEthCanisters
so the shared install_minter/install_evm_rpc can serve both the
mocked fixture (anonymous sender) and the live harness (its own
non-anonymous controller), instead of live_scan.rs hand-building
near-identical copies of both functions. Deletes those copies.

Addresses PR #11124 review comment (Medium):
#11124 (comment)
The previous commit's dedup removed live_scan.rs's local install_minter,
so "key_1" was only written once already; this replaces lib.rs's
remaining `.parse().unwrap()` literal with the named constant the
duplicate used to carry, avoiding an unnecessary fallible conversion.

Addresses PR #11124 review comments (Nit + Copilot):
#11124 (comment)
#11124 (comment)
The controller() doc wrongly blamed the anonymous-controller crash on
the controller identity. Re-tested per review: converging everything
else but keeping u128::MAX cycles (the shared mocked-fixture helper's
amount) reproduces the same crash with controller() left untouched,
confirming it is cycle-balance saturation (`AddAssign for Cycles`
saturates, so a canister already at u128::MAX cannot observe any
further addition, tripping the replica's cycle-accounting assertion on
its first live HTTPS outcall) — not the controller. controller() keeps
its own doc, now stating its real, sufficient justifications; the
cycles constraint is now documented at the add_cycles call sites.

Addresses PR #11124 review comment (Medium):
#11124 (comment)
Now that install_minter/install_evm_rpc are genuinely shared between
the two fixtures (previous commits), the enum backs both the EVM RPC
canister's install args and the minter's chain-state init args, not
just the EVM RPC side its old name implied.

Addresses PR #11124 review comment (Nit):
#11124 (comment)
…NUMBER_AT_INSTALL

Both Anvil arms already carried a one-clause reason; the Mocked arms
didn't, leaving a reader unable to tell why replaying canned mainnet
responses implies these two particular values without also reading the
enum's top-level doc.

Addresses PR #11124 review comment (Nit):
#11124 (comment)
…ses it

"every ckETH fixture creates" overstated it: the live harness creates
its 3 canisters itself, in a different order (ledger first, not
minter first) and under its own controller, then assembles this same
struct to feed the shared installers rather than creating it via
create_cketh_canisters.

Addresses PR #11124 review comment (Nit):
#11124 (comment)
@gregorydemay

Copy link
Copy Markdown
Contributor Author

🤖 Addressed all 6 review findings, one commit each:

  • 5c6bda60ec — Medium: thread the install sender through the shared install_minter/install_evm_rpc via CkEthCanisters.controller, deleting the duplicated copies in live_scan.rs.
  • c2053c6e8e — Nit + Copilot: restored ECDSA_KEY_NAME as a single shared constant; dropped the fallible .parse().unwrap().
  • f683e8c657 — Medium: corrected the wrongly-documented crash cause.
  • 7e84561945 — Nit: renamed EvmRpcBackendEthereumBackend now that it's genuinely shared.
  • 04d586b9c6 — Nit: explained the Mocked arms of ethereum_block_height/last_scraped_block_number.
  • 206bb5c905 — Nit: reworded CkEthCanisters's doc to match how each fixture actually uses it.

Cycles experiment (per the Medium-1 root-cause comment): re-tested with controller()/CanisterSettings completely untouched, only bumping the live harness's cycles from u128::from(u64::MAX) to u128::MAX — the identical Invalid cycle change: expected Added(1360000000), got Added(0) crash reproduced. This confirms cycle-balance saturation, not the controller, was the cause. controller() is kept (it has two independent, sufficient justifications — the LSO stand-in id and controller-only fetch_canister_logs), but its doc no longer attributes the crash to it; that's now explained at the add_cycles call sites.

Verification (all synchronous, --nocache_test_results where noted):

  • rustfmt — clean
  • cargo check --all-targets --all-features -p ic-cketh-test-utils -p ic-cketh-minter — clean
  • cargo clippy (pinned flags, --all-targets) — clean
  • bazel test (via ./ci/container/container-run.sh) for cketh_test, ckerc20_test, test_utils:lib_tests, deposit_from_cex — all 4 PASSED, --nocache_test_results

Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs Outdated
@gregorydemay

Copy link
Copy Markdown
Contributor Author

🤖🧐 VERDICT: READY (re-review) — 0 blockers, 0 mediums, 1 nit; CI 21 pass / 0 fail / 6 pending. Merge gate: Bazel Test All, Bazel Test All on RBE, Build IC, Cargo Build/Lint Linux, Bazel Run Fuzzers must go green before merge. Final approval is the human's — this is not an approval.

Re-review details

Per-finding resolution — all six resolved

Prev Commit Resolution
🟠 duplication 5c6bda60ec Genuinely collapsed, not moved. Both copies in live_scan.rs are deleted (−37 lines); it now assembles a CkEthCanisters and calls the shared installers. CkEthCanisters.controller is threaded into all three installers, so the sender is now handled identically everywhere instead of install_ledger hard-coding None.
🟠 wrong crash cause f683e8c657 Corrected, and the experiment (bump only the cycles, leave controller() untouched → identical crash) is the right way to have settled it. controller()'s doc now states only its two real justifications. One nit remains on the replacement wording — see inline.
🔵 ECDSA_KEY_NAME c2053c6e8e Restored as a documented shared const; also drops the gratuitously fallible "key_1".parse().unwrap().
🔵 enum name 7e84561945 EthereumBackend covers all three methods; the new doc names both facets explicitly.
🔵 uncommented Mocked arms 04d586b9c6 Both arms now carry a one-clause reason, symmetric with Anvil.
🔵 CkEthCanisters doc 206bb5c905 Accurate, and usefully spells out how the two fixtures differ in creation order and in which canisters they install.

Does EthereumBackend earn its keep now? Yes.

My earlier objection was that each method resolved to a statically-known variant at both call sites, so nothing was deduplicated. That is gone by construction: install_minter and install_evm_rpc are now single shared functions reached with both variants, so backend is a genuine parameter and each method has exactly one call site consuming a dynamic value. This is the F3 "take an enum, not a bool" form doing real work. It should stay.

Invariants re-checked after the deeper refactor

  • make_live before the minter install — holds (live_scan.rs:127:129), with the explanatory comment intact. install_evm_rpc moved from before to after create_canister(minter) (it now needs minter_id to build CkEthCanisters), but is still before make_live; canister creation order is unchanged, so both canister ids are unchanged, and the EVM RPC install schedules no outcall timers. Benign.
  • CkEthSetup::new unchanged operation-for-operation — re-verified against the pre-PR base, not just the previous head: same three creates in the same order, same u128::MAX, same ledger args, same install sequence, no added tick(). The two new indirections are value-preserving: canisters.controller is None for create_cketh_canisters, matching the previous literal None at all three install sites; and ecdsa_key_name is a plain String (lifecycle/init.rs:20), so ECDSA_KEY_NAME.to_string()"key_1".parse().unwrap(). Cold-start property preserved.
  • Live harness install sendersSome(controller()) for both minter and EVM RPC, exactly as the deleted copies did.

Nothing new introduced

No dead code or orphaned imports (EthereumNetwork, MinterInitArgs, CKETH_MINIMUM_WITHDRAWAL_AMOUNT, ETH_HELPER_CONTRACT_ADDRESS, evm_rpc_wasm all correctly dropped from live_scan.rs; minter_wasm/Nat correctly kept). No new derives. No silent fallbacks. CkEthCanisters.controller is named for the identity rather than the role it fills — cleared, since PocketIC requires an install sender to be a controller, so the two coincide by construction.

Re-ran (all fresh, --nocache_test_results)

  • //rs/ethereum/cketh/minter:integration_tests_tests/cketh_test PASSED 73.0s
  • //rs/ethereum/cketh/minter:integration_tests_tests/ckerc20_test PASSED 134.5s
  • //rs/ethereum/cketh/test_utils:lib_tests PASSED
  • //rs/ethereum/cketh/minter:deposit_from_cex PASSED 74.8s
  • cargo clippy --all-targets --all-features -p ic-cketh-test-utils -p ic-cketh-minter -- -D warnings clean

Drop the unverified "never executed as real cycles since it never goes
live" parenthetical: the experiment only established that this
harness's cycles amount triggers the crash, not why the mocked fixture
survives u128::MAX (PocketIC runs the same replica execution live or
not; the likelier distinction is the NNS subnet this harness adds and
whatever cost schedule follows from it, which was not verified either).
State only what the evidence supports. Also move the comment to sit
directly above the add_cycles call it describes, and note it applies
to minter_id's identical add_cycles below too.

Addresses PR #11124 review comment (Nit).
…self

Deletes CkErc20LiveScanSetup and its new_live(), which still ran their
own canister creation, settings, cycles and install sequencing in
parallel to CkEthSetup::new — a second fixture in substance, not just
in name.

Adds a builder on CkEthSetup (CkEthSetup::builder()) with two knobs:
with_ethereum_backend (Mocked/Anvil, already shared via EthereumBackend)
and with_live_mode, which switches on every axis the live harness needs
and off by default: an NNS subnet in addition to the fiduciary one, a
fixed non-anonymous controller, u64::MAX cycles (headroom below the
saturating balance ceiling established in the prior fix), an
uninstalled placeholder ckETH ledger, and make_live before the minter
install. CkEthSetup::new/Default keep today's behaviour unchanged,
now delegating to the same builder in its default (mocked) mode.

The live-scan-specific behaviour (Holding, SupportedToken,
credit_deposits, await_scan, balance_scan_candidates, deposit_erc20,
register_deposit_address, depositor, ckERC20 activation and token
registration) now hangs off a thin LiveBalanceScanSetup that wraps
CkEthSetup plus the anvil node, rather than reimplementing any of it.

Built on CkEthSetup rather than CkErc20Setup: the balance scan needs
only the minter and EVM RPC canister, and CkErc20Setup would drag in a
real ledger-suite-orchestrator and spawned per-token ledgers for no
benefit here.

deposit_from_cex.rs updated to construct LiveBalanceScanSetup instead
of the deleted CkErc20LiveScanSetup.
…ence

The prior split kept the same duplication under a new name: two
sibling functions each spelling out the full canister-creation/install
sequence. Collapses them into a single CkEthSetupBuilder::build with a
small number of conditionals instead:

- One new_env(live) builds the PocketIC instance for both modes:
  fiduciary subnet always, NNS subnet added and made live in live mode.
  make_live now runs before any canister of this fixture exists (no
  install has scheduled a timer yet), rather than between the EVM RPC
  and minter installs — verified against the live suite, not assumed.
- create_cketh_canisters takes the single controller: Option<Principal>
  CkEthCanisters already carried, via a create_canister(env, controller)
  helper; no separate CanisterSettings axis on the builder.
- Cycle amount is unified to u128::from(u64::MAX) for both modes,
  removing the last hardcoded fork; verified no test asserts a cycle
  balance by running the full suite.
- Canister creation order (minter first) is untouched, so it is shared
  by construction rather than by convention.

The one axis that stayed conditional: installing the real ckETH ledger
for the live harness too. Tried it; it fails outright, because the
anvil-backed deposit_from_cex Bazel target does not declare the ledger
canister Wasm as a data dependency, so load_wasm's cargo-metadata
fallback has no access to the ledger crate's directory in that
sandbox. Kept as `if !self.live { install_ledger(..) }`, documented at
the call site with the failure mode that was actually observed.

CkEthSetup::new/Default are unaffected in creation order, MINTER_ADDRESS
derivation and the cold-start property (new_env(false) never calls
make_live).
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
gregorydemay and others added 5 commits August 13, 2026 12:22
pocket_ic_builder() had exactly one call site (new_env) and its doc's
first clause restated new_env's own almost verbatim. Inlines the
PocketIcBuilder::new().with_fiduciary_subnet() chain into new_env and
deletes the function and its doc; the fiduciary-subnet rationale (the
secp256k1 key_1 the minter needs) stays stated once, on new_env's own
doc comment.

Confirmed via grep across rs/ that no other caller or re-export exists.

Addresses PR #11124 review comment:
#11124 (comment)
The Ethereum backend decides how the instance is built — a live one for
anvil, a plain one for the mocked responses — yet the builder still accepted
a caller-supplied instance, with nothing checking that it matched. Every
caller passed the very instance the mocked backend would have built anyway,
so the parameter only offered a way to get it wrong.

Build the instance from the backend alone and drop the whole chain that
threaded one in: with_env, CkEthSetup::new, new_pocket_ic, and CkErc20Setup's
env arguments. CkErc20Setup now takes the shared instance from the ckETH
fixture it builds first, which is the sharing it actually needed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ery backend

The live harness owned its canisters with a dedicated non-anonymous principal
and skipped the ckETH ledger install, so which canisters existed and who
controlled them depended on the Ethereum backend — a chain the fixture talks
to, which has no bearing on either.

Neither difference was load-bearing. The anonymous principal already controls
every canister PocketIC creates by default, which is what makes the minter's
controller-only logs readable as the anonymous caller, and the minter's
orchestrator check is a plain caller comparison that an anonymous stand-in
id satisfies. The skipped ledger install was down to the ledger Wasm missing
from the anvil-backed test target's Bazel data, so declare it there.

Canister creation and installation now match master exactly, for both
backends, and the backend decides only what it actually determines: how the
PocketIC instance is built and what the minter and EVM RPC canister are
initialised with.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The harness activated the ckERC20 feature and registered ckUSDC/ckUSDT
itself, standing in for an orchestrator: it pointed the minter's
orchestrator id at a principal it could call as, and invented a placeholder
ledger id per token that nothing ever calls. That was only to avoid the
ledger suites the balance scan does not read — but testing the deposit flow
will need them.

Wrap the anvil-backed ckETH fixture in CkErc20Setup instead, whose real
orchestrator registers the same two tokens at the same mainnet addresses the
scan reads, and drop both stand-ins.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Integrates the pocket_ic_builder inlining pushed to the branch. That change
is already present in this branch's tree, so the merged tree is unchanged;
the conflicting hunks were EnvMode and new_pocket_ic, both removed here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

rs/ethereum/cketh/test_utils/src/live_scan.rs:18

  • This paragraph still claims only the minter + EVM RPC canister are installed and that no real orchestrator/ledgers are created. However LiveBalanceScanSetup::new_live now calls CkErc20Setup::with_cketh(..).add_supported_erc20_tokens(), which installs the ledger-suite orchestrator and spawns ledger/index canisters. Update the docs to match the current behavior (or change the setup to avoid the orchestrator).
//! Only the minter and the EVM RPC canister are installed. The full ckERC20 feature is activated by
//! pointing the minter's ledger-suite-orchestrator id at a principal this harness controls, so
//! supported tokens can be registered directly via `add_ckerc20_token` without a real orchestrator
//! or any spawned ledgers — the balance scan only needs the token contract addresses in the
//! minter's state.

rs/ethereum/cketh/minter/BUILD.bazel:320

  • PR description says "no Bazel files changed", but this PR updates the deposit_from_cex Bazel target (adds wasm data deps + env vars). Please update the PR description to reflect this, since it affects how the live harness is built/executed under Bazel.
        "tests/deposit_from_cex_demo/MockUSDT.sol",
        # End-to-end balance scan on a live PocketIC + local anvil node.
        ":cketh_minter_debug.wasm.gz",
        "//rs/ethereum/ledger-suite-orchestrator:ledger_suite_orchestrator_canister.wasm.gz",
        "//rs/ledger_suite/icrc1/archive:archive_canister_u256.wasm.gz",
        "//rs/ledger_suite/icrc1/index-ng:index_ng_canister_u256.wasm.gz",
        "//rs/ledger_suite/icrc1/ledger:ledger_canister_u256.wasm.gz",
        "//rs/pocket_ic_server:pocket-ic-server",
        "@evm_rpc.wasm.gz//file",
    ],
    env = {
        "ANVIL_BIN": "$(rootpath //:anvil)",
        "MOCKUSDT_SOL": "$(rootpath tests/deposit_from_cex_demo/MockUSDT.sol)",
        "SOLC_BIN": "$(rootpath //:solc)",
        "CARGO_MANIFEST_DIR": "rs/ethereum/cketh/minter",
        "CKETH_MINTER_WASM_PATH": "$(rootpath :cketh_minter_debug.wasm.gz)",
        "EVM_RPC_CANISTER_WASM_PATH": "$(rootpath @evm_rpc.wasm.gz//file)",
        "INDEX_CANISTER_WASM_PATH": "$(rootpath //rs/ledger_suite/icrc1/index-ng:index_ng_canister_u256.wasm.gz)",
        "LEDGER_ARCHIVE_NODE_CANISTER_WASM_PATH": "$(rootpath //rs/ledger_suite/icrc1/archive:archive_canister_u256.wasm.gz)",
        "LEDGER_CANISTER_WASM_PATH": "$(rootpath //rs/ledger_suite/icrc1/ledger:ledger_canister_u256.wasm.gz)",
        "LEDGER_SUITE_ORCHESTRATOR_WASM_PATH": "$(rootpath //rs/ethereum/ledger-suite-orchestrator:ledger_suite_orchestrator_canister.wasm.gz)",
        "POCKET_IC_BIN": "$(rootpath //rs/pocket_ic_server:pocket-ic-server)",

rs/ethereum/cketh/test_utils/src/live_scan.rs:9

  • The module docs say this harness is "Unlike CkErc20Setup" because that fixture uses canned JSON-RPC mocks, but LiveBalanceScanSetup::new_live now constructs a CkErc20Setup on top of a live CkEthSetup with an anvil backend (real HTTPS outcalls). The doc should be updated to reflect that CkErc20Setup is reused here, but with a live backend rather than mocks.
//! Unlike [`crate::ckerc20::CkErc20Setup`] — which also runs on PocketIC but answers the EVM RPC
//! canister's JSON-RPC outcalls with canned canister-http mocks ([`crate::mock::MockJsonRpcProviders`])
//! — this harness runs PocketIC in *live* mode so the EVM RPC canister makes genuine outcalls
//! through the IC's HTTPS-outcalls feature, and installs it with an `overrideProvider` that
//! rewrites every provider URL to the harness' anvil node (reached over HTTP, mirroring the

@gregorydemay gregorydemay changed the title test(cketh): build the live balance-scan harness on the shared fixture test(cketh): build the live balance-scan harness on the shared fixtures Aug 13, 2026
@gregorydemay
gregorydemay marked this pull request as ready for review August 13, 2026 14:02
@gregorydemay
gregorydemay requested a review from a team as a code owner August 13, 2026 14:02
@github-actions github-actions Bot added the @defi label Aug 13, 2026
@zeropath-ai

zeropath-ai Bot commented Aug 13, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to b47bd91.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► rs/ethereum/cketh/minter/BUILD.bazel
    Add ledger suite orchestrator and ledger canister wasm paths for tests
► rs/ethereum/cketh/minter/tests/ckerc20.rs
    Update ckerc20 tests to use new setup and live balance scan adjustments
► rs/ethereum/cketh/minter/tests/deposit_from_cex.rs
    Switch to LiveBalanceScanSetup and token handling changes
► rs/ethereum/cketh/test_utils/src/ckerc20.rs
    Refactor CkErc20Setup construction and without_ckerc20_active usage
► rs/ethereum/cketh/test_utils/src/lib.rs
    Update test utils imports and Ethereum backend handling
► rs/ethereum/cketh/test_utils/src/live_scan.rs
    Introduce LiveBalanceScanSetup, token address handling adjustments, and live scan integration
► rs/ethereum/cketh/test_utils/src/live_scan.rs
    Adjust environment setup to support live Anvil backend and token provisioning
► rs/ethereum/cketh/test_utils/src/live_scan.rs
    Add EthereumBackend enum, install_args, and helper logic for live/mocked backends

deposit_from_cex failed 5/5 under CPU contention
(--runs_per_test=5 --local_test_jobs=5), each run timing out after
"100 rounds" inside CkErc20Setup's construction. new_env called
make_live before any canister existed, so the whole fixture — now
including the orchestrator spawning ledger/index canisters — was built
against an auto-progressing instance, where rounds advance on
wall-clock time rather than per call; under contention, each setup
ingress raced a deadline it did not control and lost.

Fixes it by building the whole fixture on an ordinary non-live
instance (where await_call ticks deterministically) and switching to
live outcalls only once construction is complete, right before the
balance scan needs them: LiveBalanceScanSetup::new_live now calls
env.auto_progress() after CkErc20Setup::with_cketh(..)
.add_supported_erc20_tokens() returns, instead of new_env calling
make_live up front.

auto_progress()/stop_progress() take &self, unlike make_live's &mut
self, so this works straight through the shared Arc<PocketIc> with no
restructuring of who owns the env.

Two things tried and dropped after actually being unneeded (verified,
not assumed):
- Failing pending canister-http requests before going live
  (CkEthSetup::fail_pending_https_outcalls exists for exactly this).
  The 5-way contention command passed 10/10 across two full runs
  without it, so it's left out.
- with_nns_subnet(), which make_live required for its HTTP gateway.
  Nothing here creates a gateway (canisters are driven through the
  client API; anvil is reached by the replica's canister-http
  adapter), and the suite stays green without it, so new_env no longer
  depends on EthereumBackend at all. EthereumBackend::is_live() is now
  dead and removed with it.

Timing: deposit_from_cex alone, uncached, is ~71s after this change vs
~65-76s observed for the same test before across prior verification
rounds — no material regression from going live later.

Fixes the stale live_scan.rs module doc, which predated a87674b
rebuilding the harness on CkErc20Setup and still claimed only the
minter and EVM RPC canister were installed, with no real orchestrator
or spawned ledgers.

Verification: rustfmt, cargo check --all-targets --all-features,
clippy (pinned flags, --all-targets) all clean. bazel test, all
--nocache_test_results: cketh_test, ckerc20_test, test_utils:lib_tests
all pass; deposit_from_cex passes solo and 10/10 under
--runs_per_test=5 --local_test_jobs=5 (two full runs).
After with_env was removed upstream and with_live_mode went away in
7a5682b, CkEthSetupBuilder was a one-field struct with a single
setter and two call sites: a constructor spelled in three calls
(builder(), with_ethereum_backend(..), build()).

CkEthSetup::new(env: Arc<PocketIc>) disappeared when the fixture took
over building its own PocketIC instance, freeing the name (confirmed
via grep: no remaining caller anywhere in rs/). CkEthSetup::new(backend:
EthereumBackend) now does what CkEthSetupBuilder::build did; Default
calls it with EthereumBackend::Mocked, and live_scan calls it directly
with EthereumBackend::Anvil(..).

Privacy is unchanged: new is private, and live_scan reaches it exactly
as it reached builder before, being a descendant module of the crate
root — no pub/pub(crate) added, confirmed by cargo check.

EthereumBackend itself is untouched: it still drives install_args,
ethereum_block_height and last_scraped_block_number, which are real
Mocked-vs-Anvil differences.

Verification: rustfmt, cargo check --all-targets --all-features,
clippy (pinned flags, --all-targets) all clean. bazel test, all
--nocache_test_results: cketh_test, ckerc20_test, test_utils:lib_tests
all pass; deposit_from_cex passes 5/5 under
--runs_per_test=5 --local_test_jobs=5.
Resolves the conflicts #10946 (move funded deposit addresses to a
balance-sweep queue) created with this branch's rework of the live
balance-scan harness. Both sides touched live_scan.rs.

Conflicts and how they were resolved:

- live_scan.rs imports: took master's DepositStatus, dropped
  AddCkErc20Token. This branch no longer registers supported tokens by
  hand -- CkErc20Setup does it -- so the type is unused here.

- live_scan.rs balance_scan_candidates()/candidates_in_log(): took
  master's deletion. #10946 surfaces a funded address through
  DepositStatus::AwaitingSweep instead of the [balance_scan] log line
  those helpers parsed, and the merged deposit_from_cex.rs asserts on
  that status rather than on a candidate count, so nothing calls them.

- tests/ckerc20.rs imports: kept only BTreeSet. master's new_pocket_ic
  and Arc were needed for
  CkErc20Setup::new_without_ckerc20_active(Arc::new(new_pocket_ic())),
  and on this branch the fixture builds its own PocketIC instance, so
  both call sites take no argument and new_pocket_ic no longer exists.

await_scan came through the merge with master's DepositStatus-based
body, which is what the merged deposit_from_cex.rs expects.

@mbjorkqvist mbjorkqvist left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup, @gregorydemay!

Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs
Comment thread rs/ethereum/cketh/minter/BUILD.bazel
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs Outdated
Comment thread rs/ethereum/cketh/test_utils/src/live_scan.rs
Comment thread rs/ethereum/cketh/test_utils/src/ckerc20.rs
Comment thread rs/ethereum/cketh/test_utils/src/lib.rs
Only conflict was tests/ckerc20.rs's `mod deposit_erc20` imports.
master needed new_pocket_ic and Arc for
CkErc20Setup::new_without_ckerc20_active(Arc::new(new_pocket_ic())); on
this branch the fixture builds its own PocketIC instance, so both call
sites take no argument and new_pocket_ic no longer exists. Kept
master's DEFAULT_USER_SUBACCOUNT and format_ethereum_address_to_eip_55,
which its tests use, and dropped BTreeSet, now unused after master's
rework of those tests.
- CkEthSetup::new: "anonymous controller" referred to the installed
  canisters, not the PocketIC instance; reworded.
- CkEthCanisters: "installed ... identically for every backend" was
  wrong — install_minter/install_evm_rpc take &backend and produce
  different init args (override provider, block tag, last scraped
  block). What's identical is the canister set, creation order and
  controller.
- CANISTER_CYCLES: the rationale read live-specific, but the constant
  funds every fixture, mocked included. States that funding every
  fixture the same amount is deliberate, rather than scoping the lower
  amount back to EthereumBackend::Anvil.
- EthereumBackend::Anvil: fixed two errors — the Arc clone held here
  does not outlive construction (CkEthSetup::new takes backend by
  value and drops it once install args are computed; the harness'
  own, separate Arc clone is what keeps anvil running), and "created
  and installed exactly as for Mocked" had the same init-args problem
  as CkEthCanisters' doc.
- ckerc20.rs: "its Ethereum backend decides how [the instance is
  built]" no longer holds after 7a5682b made new_env() unconditional;
  the backend only picks init args now.
- live_scan.rs module doc: states the mechanism that makes "build
  first, go live last" safe — PocketIC's auto-progress dispatch
  (ProcessCanisterHttpInternal) re-scans every canister's current
  canister_http_request_contexts() each round and dispatches whichever
  it hasn't already sent to the adapter, so outcalls issued by the
  minter's startup timers during the non-live construction phase are
  picked up for real once auto_progress() starts, not dropped.
  Confirmed by reading rs/pocket_ic_server/src/pocket_ic.rs:3792-3812.

Addresses PR #11124 review comments:
#11124 (comment)
#11124 (comment)
#11124 (comment)
#11124 (comment)
#11124 (comment)
#11124 (comment)
SupportedToken::ALL and credit_deposits hardcoded ckUSDC/ckUSDT, while
add_supported_erc20_tokens() registers whatever
ic_ledger_suite_orchestrator_test_utils::supported_erc20_tokens()
returns. The two could silently disagree: a third token added upstream
would get no code on anvil, and per credit_deposits' own comment, a
token without code reverts the whole scan even for holdings that don't
involve it — surfacing as a 180s hang with a message pointing nowhere.

Deletes SupportedToken and passes the orchestrator's own Erc20Token
(AddErc20Arg) through instead, so the set that gets code on anvil IS
the registered set, by construction rather than convention:

- Holding<'a> borrows &'a Erc20Token rather than owning a SupportedToken
  copy — the lifetime stayed simple enough at every call site that a
  cloned Erc20Token was never needed.
- credit_deposits takes each holding's address off its own token and
  iterates self.supported_erc20_tokens() instead of SupportedToken::ALL.
- deposit_erc20/register_deposit_address/await_scan (widened by the
  master merge to take a token) take &Erc20Token too.
- A new supported_erc20_tokens() accessor exposes the registered list,
  so deposit_from_cex.rs destructures
  `let [usdc, usdt] = setup.supported_erc20_tokens() else { panic!(..) }`
  — the let-else doubling as the loud arity check.
- USDT_ERC20_CONTRACT_ADDRESS is gone; it only existed to feed the enum.

Checked the EIP-55 case rather than assuming it: the minter's reported
erc20_contract_address is compared against token.contract.address
directly (no re-normalizing through Address::to_string()), and the
live suite's 10/10 contention passes confirm the two already agree.

Addresses PR #11124 review comment:
#11124 (comment)
@gregorydemay

Copy link
Copy Markdown
Contributor Author

🤖 Addressed all 9 open threads, in two commits:

  • 31c0c76045 — doc accuracy fixes: CkEthSetup::new (anonymous controller scope), CkEthCanisters ("identically for every backend" → same order, different init args), CANISTER_CYCLES (states funding every fixture the same amount is deliberate), EthereumBackend::Anvil (Arc lifetime + init-args wording), ckerc20.rs ("its Ethereum backend decides how"), and the live_scan.rs module doc (added the missing "why build-first-go-live-last is safe" paragraph, mechanism verified by reading rs/pocket_ic_server/src/pocket_ic.rs:3792-3812).
  • 407406798d — the substantive fix: deleted SupportedToken entirely per Greg's call (supersedes my "assert lengths agree" suggestion). Holding, deposit_erc20, register_deposit_address, await_scan now take the orchestrator's own Erc20Token (the lifetime approach worked cleanly at every call site, no cloning needed), and a new supported_erc20_tokens() accessor plus let [usdc, usdt] = ... else { panic!(..) } in the test give the loud arity check. Verified the EIP-55 comparison empirically rather than assuming — direct string comparison against token.contract.address works, confirmed by the live suite passing 10/10 under contention.

BUILD.bazel size = "medium" — no change, replied with the measured numbers (~72–73s solo, 10/10 under contention across two runs, RBE passed on 3cb7de4). CkErc20Setup's five-entry-point collapse — declined as out of scope, noted as a follow-up.

PR description rewritten: no longer claims the chain under test decides liveness (it's decided after construction via auto_progress()), and no longer claims mocked-fixture behaviour is fully unchanged (calls out the u128::MAXu64::MAX cycles change).

Verification (all synchronous, uncached): rustfmt, cargo check --all-targets --all-features, clippy (pinned flags + --all-targets) all clean. cketh_test 27/27, ckerc20_test 42/42, test_utils:lib_tests pass. deposit_from_cex under --runs_per_test=5 --local_test_jobs=5: 10/10 across two full runs, ~72s avg each.

@gregorydemay
gregorydemay enabled auto-merge August 17, 2026 12:14
#11145 (configure and expose the sweeper contract address) added
MinterInitArgs::ethereum_sweeper_contract_address, touching the two
pre-refactor install_minter functions this branch had already replaced
with a single shared one -- so git spliced master's version of them
into CkEthCanisters (lib.rs) and into the deleted live-scan harness
(live_scan.rs).

Took this branch's side in both files and carried the new field across
by hand, to the one surviving install_minter, defaulting to None as
master does for every non-sweeper fixture.
@gregorydemay
gregorydemay added this pull request to the merge queue Aug 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Aug 17, 2026
@gregorydemay
gregorydemay added this pull request to the merge queue Aug 18, 2026
Merged via the queue into master with commit e0823e1 Aug 18, 2026
40 checks passed
@gregorydemay
gregorydemay deleted the ic_DEFI-2262_8_live-scan-setup branch August 18, 2026 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants